Repository navigation
speed up bounds-checked element access and cache vector lengths - #1511
kevinushey wants to merge 3 commits into
Conversation
| @@ -55,7 +66,7 @@ namespace traits{ | |||
| void check_index(R_xlen_t i) const { | |||
| #ifndef RCPP_NO_BOUNDS_CHECK | |||
| if (i >= size) { | |||
There was a problem hiding this comment.
Maybe the likely/unlikely macro technique used in the kernel helps too in these cases?
There was a problem hiding this comment.
I see there are C++ attributes too, but that means C++20.
There was a problem hiding this comment.
Good thought -- I checked, and it turns out __builtin_expect would be redundant with what's already here. Both GCC and Clang treat any branch leading to a call of a cold function as unlikely, so the __attribute__((noinline, cold)) on warn_index_out_of_bounds() already provides the same hint as if (unlikely(i >= size)).
To confirm, I compiled a few hot loops (a reduction, a fill, and a sugar expression over NumericVector) with this branch vs. this branch plus __builtin_expect(!!(i >= size), 0) in check_index(), using Apple clang 21 and gcc 16 at -O2:
- gcc: byte-for-byte identical output apart from swapped compare operands. gcc also drops the bounds check entirely from the reduction loop here, since the loop bound and the checked size are now the same cached value.
- clang: the only difference is which way the loop exit block falls through; the instruction count differs by one and the sugar function is identical.
- Timings on a 1e6-element vector were within noise across the two variants for both compilers.
Interestingly, keeping __builtin_expect but removing cold does change gcc's output: with cold, gcc keeps hot-loop values in caller-saved registers and spills around the (never-taken) warning call; without it, it reserves callee-saved registers in the prologue instead. So cold is doing slightly more than expect alone would.
Portability would be the same either way, since __builtin_expect needs the same __GNUC__ guard, and MSVC has neither (the portable spelling is C++20's [[unlikely]], which we can't require). So I'll leave it as is.
Caching the length in proxy_cache and the column count in Matrix added data members, changing the layout of List, CharacterVector and Matrix. Packages pass these by reference across shared libraries compiled against different Rcpp versions: rust hands a List to revdbayes through an XPtr function pointer, and lite, threshr and evmissing reach the same path. In the 2026-10-06 reverse-dependency run revdbayes read a size that rust never wrote and failed name lookup with 'Index out of bounds'. ncol() is now derived from the cached length and nrow(), and proxy_cache reads the length on demand; only the cold warning helper and the cached atomic-vector length remain.
2 participants
Closes #1510.
The per-element bounds check added in #1310 was noticeably slowing down element access in tight loops, including sugar expressions. This PR keeps the check (and its warning) but makes it cheap:
inst/include/Rcpp/vector/traits.h: the warning now lives in anoinline/coldhelper (warn_index_out_of_bounds()), so tinyformat and the 8 KB message buffer are no longer inlined into every loop body that reads an element. Both caches gain aget_size().inst/include/Rcpp/vector/Vector.h:size()/length()return the length already cached byr_vector_cacheinstead of callingRf_xlength()(an opaque call into libR) each time.update()already runs on everyset__(), so the cache is refreshed whenever the underlying SEXP changes. ForList/CharacterVector/ExpressionVector(proxy_cache) the length is still read on demand; see "Object layout" below.inst/include/Rcpp/vector/Matrix.h:ncol()/cols()(and soMatrixRow::size()) are derived from the cached length andnrow()rather than callingRf_isMatrix()+Rf_getAttrib()each time. Only an empty matrix (which may still have columns) reads thedimattribute.Object layout
An earlier version of this PR cached the length in
proxy_cacheand the column count inMatrixas new data members. That changedsizeof(List)andsizeof(CharacterVector)from 24 to 32 bytes and added a member toMatrix. The 2026-10-06 reverse-dependency run caught this: rust passes aconst List&into revdbayes through an XPtr function pointer, and with rust prebuilt against released Rcpp, revdbayes read asizethat rust had never written and failed name lookup withIndex out of bounds: [index='N0']. Rebuilding rust from source made it pass, confirming the cause. lite, threshr and evmissing reach the same path through revdbayes's compiled posteriors; bang and fitdistcp use rust with R-level log posteriors and are not affected.So the layout of
Vector,Matrixand their caches is a de-facto ABI between packages. This version adds no data members:sizeof()of every vector and matrix class matches released Rcpp 1.1.2, and the header comments inMatrix.handtraits.hnow say why. revdbayes 1.5.7 checks clean against this branch with the April CRAN binary of rust.Benchmarks
R 4.6.1, arm64 macOS; the same functions compiled against
mastervs this branch. Results are identical between the two.x + y(1e7)x * y + 2 * x - y(1e7)for (i < x.size()) s += x[i](1e7)CharacterVectorloop,x[i] == NA_STRING(1e7)Listloop,Rf_isNull(x[i])(1e6)j < m.ncol()(1e6)MatrixRowloop withj < r.size()(1e6)The
CharacterVectorandListloops gain less than in the earlier version because their length is no longer cached (see above); the cold warning path accounts for the remaining improvement.Simple sugar expressions like
x + yare still slower than withRCPP_NO_BOUNDS_CHECK, since the (never-taken) branch still blocks vectorization. Letting sugar read plain vector operands without a per-element check would close that gap, but that's a larger change to the sugar machinery, so I've left it for a follow-up.Notes
SETLENGTH()/R_resizeVector(). Nothing in Rcpp does this, andMatrix::nrow()already had the same caching behavior.RunAllRcppTests=yes(1623 tests, 0 failures).